Breaking: Implement parseForESLint() function #412

JamesHenry · 2017-12-12T00:22:19Z

This PR implements a parseForESLint() function.

This will be preferred by ESLint, and so we can use it to implement tweaks to the AST which are only required in an ESLint context. The idea is that this will be used only where absolutely necessary.

The most pressing case is that of a method's value (FunctionExpression) having no body in some valid situations in TypeScript.

For now, the simplest solution is just to namespace the node.type as TSEmptyBody${node.type} only in the case that we are parsing for ESLint.

Prettier's usage will be completely unaffected because it will continue to use parse().

Fixes #400 #389 #402 #253

🎉

Other minor notes:

I updated the README, there were some out of date issues in there
I removed old babel-eslint issue numbers from that spec file as they were confusing
Removed the erroneously committed yarn.lock file

j-f1 · 2017-12-12T00:28:01Z

Are you sure TSEmptyBody${foo} is the best solution? IMHO the FunctionDeclaration variant should be TSFunctionOverloadDeclaration, and the MethodDeclaration variant should be TSMethodOverloadDeclaration, since that is what they are used for.

JamesHenry · 2017-12-12T10:39:27Z

@j-f1 I was personally just thinking how the name is very unimportant at this stage. The only stakeholders that could possibly care about these particular node names are custom ESLint TypeScript rule authors (I know you are one of them :D) and all of that goes through eslint-plugin-typescript right now. Any non-ESLint users of this parser won't even come across those nodes.

Having a more situational name like TSFunctionOverloadDeclaration, describing what it does rather than what it is, would simply require quite a bit more code to cover all the cases.

This seemed like the more straightforward starting point, and we could certainly change it in the future if it makes sense to.

mysticatea · 2017-12-12T10:58:48Z

In my understand, this PR adds 2 node types:

TSEmptyBodyFunctionExpression
TSEmptyBodyFunctionDeclaration

Looks good to me.

By the way, I think that some cases should be throw syntax errors. For example:

const obj = {
    a()
}

TypeScript playground shows a syntax error: https://www.typescriptlang.org/play/index.html#src=const%20obj%20%3D%20%7B%0D%0A%20%20%20%20a()%0D%0A%7D
But typescript-eslint-parser does not realize this syntax error: http://astexplorer.net/#/gist/e2dad04ac8feee4f9e19e53872aeef2b/e28c438e3eec65e47d9d6a7be45627b85f77aa76

JamesHenry · 2017-12-12T11:07:06Z

Yes, this is the case in general with the TypeScript compiler (which powers this parser). It is hyper lenient during the parsing phase because it has language services which come later in the pipeline to provide feedback to developers whilst they author TypeScript.

Adding our own strictness on top of that would need to come later in a separate PR, but it might be worth doing at some point.

j-f1 · 2017-12-12T11:09:01Z

Having a more situational name like TSFunctionOverloadDeclaration, describing what it does rather than what it is, would simply require quite a bit more code to cover all the cases.

In what other cases would you get one of these bodyless functions?

JamesHenry · 2017-12-12T11:18:55Z

E.g.

interface I {
    func(): string;
}

declare function myFunc(): boolean;

abstract class C {
    abstract foo(): number;
}

kaicataldo · 2017-12-12T16:18:05Z

tests/integration/from-babel-eslint.spec.js

-            }
-        );
-    });
+    // it("no-implicit-globals in module", () => {


Just for my own knowledge, why are these tests commented out?

I haven't solved them yet. They need to be solved by a custom Resolver I think, but I didn't want to lose them when I cut the custom resolver out of this PR in favour of this new flag.

It would be fair enough to request I take them out of the PR, but laziness made me want to keep them in as reminders 😄

j-f1 · 2017-12-12T16:24:13Z

@JamesHenry

interface I {
    func(): string; // TSMethodSignature
}

declare function myFunc(): boolean; // DeclareFunction

abstract class C {
    abstract foo(): number; // TSAbstractMethodDeclaration with FunctionExpression value
}

How about TSMethodAnnotation and TSFunctionAnnotation as types instead?

not-an-aardvark

LGTM aside from one comment.

not-an-aardvark · 2017-12-12T16:25:48Z

parser.js

@@ -177,6 +185,12 @@ exports.version = require("./package.json").version;

 exports.parse = parse;

+exports.parseForESLint = function parseForESLint(code, options) {
+    options.parseForESLint = true;


I think it would be best to avoid mutating the options object provided by the user. (This was also a problem in eslint/js#329.)

In which case I would be forced to change the signature of parse() to accept an optional third argument. There's no reason that would be an issue, right?

I don't think it would be an issue.

If you don't want the change to be visible to the public API, you could create a helper function, e.g. parseMaybeForESlint(code, options, internalOptions), and have parse call parseMaybeForESLint(code, options, { isForESLint: false }) and have parseForESLint call parseMaybeForESLint(code, options, { isForESLint: true }).

What about just defensively cloning the options so the user can't see the mutation?

You could also do options = Object.assign({}, options, { parseForESLint: true }). Or insert options = Object.create(options) before this line.

It seems strange to have parseForESLint be mixed with the user-provided options object. For example, if the user added parseForESLint: true to their options object for some reason, they could cause strange behavior.

JamesHenry · 2017-12-13T02:22:57Z

@not-an-aardvark PTAL at my latest commit b273fca

JamesHenry · 2017-12-13T10:49:37Z

@j-f1 eslint-plugin-typescript is the only possible stakeholder in those node types right now, so we can discuss again if it becomes an issue with that, and they can easily be changed. I really wanted to get this in ASAP and feel TSEmptyBodyFunctionExpression TSEmptyBodyFunctionDeclaration are at least very clear as a starting point, given they are replacing FunctionExpression and FunctionDeclaration.

Breaking: Implement parseForESLint() function

a65c8ca

JamesHenry mentioned this pull request Dec 12, 2017

Use TSAbstractFunctionExpression for abstract methods. #410

Closed

JamesHenry requested review from ilyavolodin, kaicataldo and not-an-aardvark December 12, 2017 00:29

kaicataldo reviewed Dec 12, 2017

View reviewed changes

not-an-aardvark suggested changes Dec 12, 2017

View reviewed changes

Refactor application of isParseForESLint logic

b273fca

not-an-aardvark approved these changes Dec 13, 2017

View reviewed changes

JamesHenry merged commit aec31cb into master Dec 13, 2017

JamesHenry deleted the parse-for-eslint branch December 13, 2017 10:46

This was referenced Dec 13, 2017

no-redeclare throws errors on overloaded functions #402

Closed

Empty body function cuase errors for several eslint rules #253

Closed

Overloaded method signatures failing on certain eslint rules #389

Closed

macklinu mentioned this pull request Mar 12, 2018

Chore: Update typescript-eslint-parser and typescript bradzacher/eslint-plugin-typescript#115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breaking: Implement parseForESLint() function #412

Breaking: Implement parseForESLint() function #412

JamesHenry commented Dec 12, 2017 •

edited

Loading

j-f1 commented Dec 12, 2017 •

edited

Loading

JamesHenry commented Dec 12, 2017

mysticatea commented Dec 12, 2017

JamesHenry commented Dec 12, 2017

j-f1 commented Dec 12, 2017

JamesHenry commented Dec 12, 2017

kaicataldo Dec 12, 2017

JamesHenry Dec 12, 2017 •

edited

Loading

j-f1 commented Dec 12, 2017

not-an-aardvark left a comment

not-an-aardvark Dec 12, 2017

JamesHenry Dec 12, 2017

not-an-aardvark Dec 12, 2017

platinumazure Dec 12, 2017

j-f1 Dec 12, 2017

not-an-aardvark Dec 12, 2017

JamesHenry commented Dec 13, 2017

JamesHenry commented Dec 13, 2017

Breaking: Implement parseForESLint() function #412

Breaking: Implement parseForESLint() function #412

Conversation

JamesHenry commented Dec 12, 2017 • edited Loading

j-f1 commented Dec 12, 2017 • edited Loading

JamesHenry commented Dec 12, 2017

mysticatea commented Dec 12, 2017

JamesHenry commented Dec 12, 2017

j-f1 commented Dec 12, 2017

JamesHenry commented Dec 12, 2017

kaicataldo Dec 12, 2017

Choose a reason for hiding this comment

JamesHenry Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

j-f1 commented Dec 12, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark Dec 12, 2017

Choose a reason for hiding this comment

JamesHenry Dec 12, 2017

Choose a reason for hiding this comment

not-an-aardvark Dec 12, 2017

Choose a reason for hiding this comment

platinumazure Dec 12, 2017

Choose a reason for hiding this comment

j-f1 Dec 12, 2017

Choose a reason for hiding this comment

not-an-aardvark Dec 12, 2017

Choose a reason for hiding this comment

JamesHenry commented Dec 13, 2017

JamesHenry commented Dec 13, 2017

JamesHenry commented Dec 12, 2017 •

edited

Loading

j-f1 commented Dec 12, 2017 •

edited

Loading

JamesHenry Dec 12, 2017 •

edited

Loading